Skip to content

Conversation

@lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented May 2, 2025

Split out from #5043 due to apparent breakage in the upgrade

@lihaoyi lihaoyi marked this pull request as ready for review May 2, 2025 22:48
@lihaoyi
Copy link
Member Author

lihaoyi commented May 3, 2025

@vaslabs seems like upgrading OS-Lib is hitting issues in the android test. Do you know what zip file it is trying to unzip? If we could get the zip standalone outside of the mill test suite, that would make it much easier to debug the os-lib issue upstream

@vaslabs
Copy link
Contributor

vaslabs commented May 3, 2025

I'll check, give me some time
In the past there was an issue with nested directories when unzipping after a version bump, might be the same

EDIT: Issue looks different now, I'm investigating

@vaslabs
Copy link
Contributor

vaslabs commented May 3, 2025

@lihaoyi looks like the permissions are not set properly

image

image

basically androidx is a directory and should have x flag in order to be listable, must be some change on how permissions are set to the unzipped directories in os lib

image

EDIT: don't know if it helps, but it's the first directory in the directory tree inside a zip, it looks like the directories after that are fine

EDIT2: looks like it happened only for androidx test, maybe there's something peculiar about this particular zip file

EDIT3: There are multiple jars being extracted to classes/ and classes/androidx so maybe that contributes to the issue (these files are the ones we extract from aar)

EDIT4: I'll need more time, need to get os lib and build it locally to do more tests

@vaslabs
Copy link
Contributor

vaslabs commented May 3, 2025

looks like the androidx junit 1.2.1 doesn't have any unix file attributes (zipinfo -v) which may contribute to that

  androidx/

  offset of local header from start of archive:   139
                                                  (000000000000008Bh) bytes
  file system or operating system of origin:      MS-DOS, OS/2 or NT FAT
  version of encoding software:                   1.0
  minimum file system compatibility required:     MS-DOS, OS/2 or NT FAT
  minimum software version required to extract:   1.0
  compression method:                             none (stored)
  file security status:                           not encrypted
  extended local header:                          no
  file last modified on (DOS date/time):          2010 Jan 1 00:00:00
  32-bit CRC value (hex):                         00000000
  compressed size:                                0 bytes
  uncompressed size:                              0 bytes
  length of filename:                             9 characters
  length of extra field:                          0 bytes
  length of file comment:                         0 characters
  disk number on which file begins:               disk 1
  apparent file type:                             binary
  non-MSDOS external file attributes:             000000 hex
  MS-DOS file attributes (00 hex):                none

  There is no file comment.

I'll check if it's originally like that or we mess it up somehow

EDIT: it's originally like that, others seem to have proper unix attributes that look like:

Central directory entry #4:
---------------------------

  androidx/

  offset of local header from start of archive:   200
                                                  (00000000000000C8h) bytes
  file system or operating system of origin:      Unix
  version of encoding software:                   2.0
  minimum file system compatibility required:     MS-DOS, OS/2 or NT FAT
  minimum software version required to extract:   2.0
  compression method:                             deflated
  compression sub-type (deflation):               normal
  file security status:                           not encrypted
  extended local header:                          no
  file last modified on (DOS date/time):          1980 Feb 1 00:00:00
  32-bit CRC value (hex):                         00000000
  compressed size:                                2 bytes
  uncompressed size:                              0 bytes
  length of filename:                             9 characters
  length of extra field:                          0 bytes
  length of file comment:                         0 characters
  disk number on which file begins:               disk 1
  apparent file type:                             binary
  Unix file attributes (040755 octal):            drwxr-xr-x
  MS-DOS file attributes (10 hex):                dir 

  There is no file comment.

@lihaoyi
Copy link
Member Author

lihaoyi commented May 3, 2025

Got it, it could be a weakness in the new zip implementation that @kiendang worked on, but probably fixable

@vaslabs
Copy link
Contributor

vaslabs commented May 3, 2025

Got it, it could be a weakness in the new zip implementation that @kiendang worked on, but probably fixable

Yeah it's a very peculiar use case, I checked the newest version of androidx junit, has the same issue.

Let me know if you need any help to try things to fix it or how to reproduce. The problematic jar can be found here

(classes.jar in the aar)

@kiendang
Copy link
Contributor

kiendang commented May 4, 2025

If someone points me to the code where the jar is unzipped, I can take a look.

@lihaoyi
Copy link
Member Author

lihaoyi commented May 4, 2025

@kiendang the problematic callsite is

, the problematic zip should be the one @vaslabs linked. I think you should be able to reproduce the problem outside of Mill to fix it

@kiendang
Copy link
Contributor

kiendang commented May 7, 2025

The problem is classes.jar contains zip entries for directories with permission information. However the permission is missing x

Examples:

tracing-1.1.0/classes.jar

zipinfo /home/kien/mill/out/example/androidlib/java/1-hello-world/local/server/testForked.dest/sandbox/run-1/out/app/it/androidTransformAarFiles.dest/transform/tracing-1.1.0/classes.jar
Archive:  /home/kien/mill/out/example/androidlib/java/1-hello-world/local/server/testForked.dest/sandbox/run-1/out/app/it/androidTransformAarFiles.dest/transform/tracing-1.1.0/classes.jar
Zip file size: 3967 bytes, number of entries: 6
-rw-rw-rw-  0.0 unx        0 b- stor 81-Jan-01 01:01 androidx/
-rw-rw-rw-  0.0 unx        0 b- stor 81-Jan-01 01:01 androidx/tracing/
-rw-rw-rw-  0.0 unx     4693 b- defN 81-Jan-01 01:01 androidx/tracing/Trace.class
-rw-rw-rw-  0.0 unx      719 b- defN 81-Jan-01 01:01 androidx/tracing/TraceApi18Impl.class
-rw-rw-rw-  0.0 unx     1093 b- defN 81-Jan-01 01:01 androidx/tracing/TraceApi29Impl.class
-rw----     2.4 fat        6 b- stor 81-Jan-01 01:01 META-INF/androidx.tracing_tracing.version

junit-1.2.1/classes.jar

zipinfo /home/kien/mill/out/example/androidlib/java/1-hello-world/local/server/testForked.dest/sandbox/run-1/out/app/it/androidTransformAarFiles.dest/transform/junit-1.2.1/classes.jar
Archive:  /home/kien/mill/out/example/androidlib/java/1-hello-world/local/server/testForked.dest/sandbox/run-1/out/app/it/androidTransformAarFiles.dest/transform/junit-1.2.1/classes.jar
Zip file size: 10824 bytes, number of entries: 13
-rw----     2.0 fat       24 bX defN 10-Jan-01 00:00 META-INF/ext_junit_java_androidx_test_ext_junit-junit_kt.kotlin_module
-rw----     1.0 fat        0 b- stor 10-Jan-01 00:00 androidx/
-rw----     1.0 fat        0 b- stor 10-Jan-01 00:00 androidx/test/
-rw----     1.0 fat        0 b- stor 10-Jan-01 00:00 androidx/test/ext/
-rw----     1.0 fat        0 b- stor 10-Jan-01 00:00 androidx/test/ext/junit/
-rw----     1.0 fat        0 b- stor 10-Jan-01 00:00 androidx/test/ext/junit/rules/
-rw----     2.0 fat     1810 bl defN 10-Jan-01 00:00 androidx/test/ext/junit/rules/AppComponentFactoryRule.class
-rw----     2.0 fat      614 bl defN 10-Jan-01 00:00 androidx/test/ext/junit/rules/ActivityScenarioRule$Supplier.class
-rw----     2.0 fat     4330 bl defN 10-Jan-01 00:00 androidx/test/ext/junit/rules/ActivityScenarioRule.class
-rw----     2.0 fat     2789 bl defN 10-Jan-01 00:00 androidx/test/ext/junit/rules/DeleteFilesRule$1.class
-rw----     2.0 fat     3262 bl defN 10-Jan-01 00:00 androidx/test/ext/junit/rules/DeleteFilesRule.class
-rw----     1.0 fat        0 b- stor 10-Jan-01 00:00 androidx/test/ext/junit/runners/
-rw----     2.0 fat     6685 bl defN 10-Jan-01 00:00 androidx/test/ext/junit/runners/AndroidJUnit4.class
13 files, 19514 bytes uncompressed, 8630 bytes compressed:  55.8%

os.unzip does nothing wrong here. It sees androidx/ with -rw-rw-rw- and thus creates the androidx directory with drw-rw-rw- .

I tried using the unzip command on linux and the same problem occurs.

Previously it works because os.unzip did not use the permission information when unzipping files. We can add a flag to os.unzip to ignore permissions.

@vaslabs
Copy link
Contributor

vaslabs commented May 7, 2025

strange, for me the unzip command (Fedora 41 6.13.12-200.fc41.x86_64) gives proper permissions for directories

image

image

EDIT: tracing has the same issue with local unzip yeah

image

I'm personally ok with either an opt-in or opt out option for android's case, doesn't really matter

@lihaoyi
Copy link
Member Author

lihaoyi commented May 7, 2025

Thanks for the investigation @kiendang. So for the given classes.jar, we're seeing the crash inside OS-Lib's os.unzip code, so we don't really get a chance to manually fix up the permissions issue in Mill:

[4611-0]       os.makeDir$all$.apply(FileOps.scala:55)
[4611-0]       os.unzip$.apply$$anonfun$6(ZipOps.scala:363)
[4611-0]       scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
[4611-0]       scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
[4611-0]       scala.collection.IterableOnceOps.foreach(IterableOnce.scala:619)
[4611-0]       scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:617)
[4611-0]       scala.collection.AbstractIterable.foreach(Iterable.scala:935)
[4611-0]       scala.collection.IterableOps$WithFilter.foreach(Iterable.scala:905)
[4611-0]       os.unzip$.apply(ZipOps.scala:393)
[4611-0]       mill.androidlib.AndroidAppModule.androidLibsClassesJarMetaInf$$anonfun$1$$anonfun$1$$anonfun$2(AndroidAppModule.scala:227)
[4611-0]       scala.collection.immutable.List.flatMap(List.scala:294)
[4611-0]       scala.collection.immutable.List.flatMap(List.scala:79)
[4611-0]       mill.androidlib.AndroidAppModule.androidLibsClassesJarMetaInf$$anonfun$1$$anonfun$1(AndroidAppModule.scala:225)
[4611-0]       mill.define.NamedTask.evaluate(Task.scala:248)
[4611-0]       mill.define.NamedTask.evaluate$(Task.scala:236)
[4611-0]       mill.define.TargetImpl.evaluate(Task.scala:382)

I guess the fix would be to make OS-Lib add the "executable" bit to all folders regardless of the zip file's permissions, so at least we are able to create the nested directory contents without crashing?

@kiendang
Copy link
Contributor

kiendang commented May 7, 2025

Yup maybe "os.unzip did nothing wrong" wasn't totally accurate. It created the outer directory with the specified permissions then crashed when attempted to unzipping the directory contents.

Adding OWNER_EXECUTE to directory perms sounds like a good default.

@lihaoyi
Copy link
Member Author

lihaoyi commented May 7, 2025

Got it. @kiendang could you send a PR?

@kiendang
Copy link
Contributor

kiendang commented May 7, 2025

com-lihaoyi/os-lib#387

@lefou
Copy link
Member

lefou commented May 7, 2025

Shouldn't we first create the content, and then apply the permissions? If that means, the directory won't be accessible any longer, so be it.

@kiendang
Copy link
Contributor

kiendang commented May 7, 2025

Shouldn't we first create the content, and then apply the permissions? If that means, the directory won't be accessible any longer, so be it.

Yea I think that's how the unzip command does it. I decided to unzip the parents before their contents so that I don't have to revisit the parents later to set their permissions. I didn't think of this case.

However I think in this case with classes.jar the directories are not meant to have the permissions without x though, so even if we unzip children first, would still need a restorePermissions = false flag for this case.

lihaoyi pushed a commit to com-lihaoyi/os-lib that referenced this pull request May 9, 2025
to prevent crash in cases where directories is missing READ/EXECUTE
permission

Added a test that
- creates a zip file with directories without READ/EXECUTE permissions
- `os.unzip` everything successfully
- re-adds READ and EXECUTE permissions to directories

see my and @lefou's comments
#387 (comment)
com-lihaoyi/mill#5048 (comment)

Tested with the android test. Here is the permission fix at the call
site (`AndroidApModule.scala`)

com-lihaoyi/mill@main...kiendang:mill:fix-android-classesjar-unzip#diff-49ebd9c68d1348785194e15a80d62f0845c26386e722d3f5d43429238bbf7616

@lihaoyi @vaslabs
@lihaoyi
Copy link
Member Author

lihaoyi commented May 9, 2025

Updated this PR with @kiendang's latest updates to OS-Lib, let's see how it goes

@lihaoyi lihaoyi merged commit 217acb9 into com-lihaoyi:main May 9, 2025
9 checks passed
@lihaoyi
Copy link
Member Author

lihaoyi commented May 9, 2025

@kiendang your patch seems to fix things, thanks! CC @arturaz this should fix the failure on your PR

@lefou lefou added this to the 1.0.0-RC1 milestone May 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants